Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port CoreCLR's TypeNameBuilder to C#, and use it in Mono too #33701

Merged

Conversation

alexischr
Copy link
Contributor

@alexischr alexischr commented Mar 18, 2020

Port CoreCLR's TypeNameBuilder to C#, and use it in Mono too

Mono's System.Reflection.Emit creates type names that fail to be normalized or shaped in all ways that CoreCLR does.

Port CoreCLR's mixed-mode thread-unsafe implementation to thread-safe C#, and start using it in Mono for names in TypeBuilder.

Fixes issues with e.g. null-delimited type names being passed to different Reflection.Emit builders. Contributes to #2389.

@jkotas
Copy link
Member

jkotas commented Mar 18, 2020

Would it make sense to switch CoreCLR to use the fully managed implementation as well and get rid of the mixed-mode implementation?

ERROR = 0x0100,
}

ParseState m_parseState;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParseState does not seem to be used for anything. Delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you mean? CheckParseState checks against this value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CheckParseState checks this value, but the only thing that is done as a result of this test is setting this value to a different value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be debug-only and only used for asserts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plan what to do with the parseState ? I would delete it completely. The asserts that it is used for are not very valuable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep it - The asserts helped me catch a porting typo in ToString, and they make some functions invariants explicit. I did remove the "error" state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it debug-only at least so that we do not have to carry it in shipping builds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, make the names follow the C# coding style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I make this debug-only? Is using System.Diagnostics.Debug not enough?

Copy link
Member

@jkotas jkotas Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still going to be the enum type, an empty method and 10+ calls to this method, and the code that sets the field...

@alexischr
Copy link
Contributor Author

The test will not actually succeed with this PR, there are two other issues. Enabling it will not be part of the final PR

@alexischr
Copy link
Contributor Author

Would it make sense to switch CoreCLR to use the fully managed implementation as well and get rid of the mixed-mode implementation?

It probably would, my first impression is that there isn't much use of this code from the native side (except perhaps those TypeString functions)

@alexischr
Copy link
Contributor Author

Thank you for the feedback! This is a WIP:

  • I have not yet formatted the code, a lot is sourced from CoreCLR but I was following the Mono style. @marek-safar which style do you want for new files? It looks like there's desire to share this code in both runtimes so I think CoreCLR-style.

  • Some of the code structure is dubious but not clearly from the beginning, e.g. Clear() seemed to be set up for reusing the internal C++ class, but there was no actual use of it that way AFAICT. I have some more cleaning to make.

  • This looks like it can be refactored to be thread-safe by not using the TypeNameBuilderImpl that is stored in a field, so I will probably do that.

@marek-safar
Copy link
Contributor

there's desire to share this code in both runtimes

In that case, it should follow the existing code style of shared SPC sources. I don't know what is the intention for CoreCLR but maybe it could land in the shared location instead of mono from the start

@alexischr alexischr force-pushed the mono-port-coreclr-typenamebuilder branch 2 times, most recently from 3dfd0ee to c16b15c Compare March 18, 2020 21:26
@alexischr
Copy link
Contributor Author

Update:

  • Applied feedback, moved/renamed file to shared S.P.C location
  • This is now thread-safe; TypeNameBuilderImpl now contains all the state and a new one is used for each call to ToString. The class interface to TypeNameBuilder has not changed.
  • Experimenting with using it in CoreCLR (not done properly, have not removed files, just want to see CI's response)

@alexischr
Copy link
Contributor Author

Looks like System.Collections.Stack isn't in CoreCLR's CoreLib (while System.Collections.Generic.Stack is missing from Mono's)

@jkotas
Copy link
Member

jkotas commented Mar 19, 2020

TypeNameBuilderImpl now contains all the state and a new one is used for each call to ToString. The class interface to TypeNameBuilder has not changed.

I do not see a reason for this split. Can all state and methods from TypeNameBuilderImpl be moved to be private state and methods on TypeNameBuilder ?

@alexischr alexischr force-pushed the mono-port-coreclr-typenamebuilder branch from b6faac4 to 6af771f Compare April 1, 2020 14:28
@alexischr
Copy link
Contributor Author

@jkotas I think all issues are resolved except how to keep the parser state checks. I like them because they were part of the original and this is still a port of that code, which has now changed considerably and could do with some checking. I also don't think they will have a real impact in a program's performance. But I understand they add some IL and an extra class to CorLib, so increase its size.

Is the only way you would have them is in #if DEBUG blocks? That is probably too ugly because there's a bunch of single statements, and I will probably just take them out then. Would it be agreeable to leave them as comments?

@alexischr
Copy link
Contributor Author

alexischr commented Apr 1, 2020

I can also ask the compiler for aggressive inlining for CheckParseState, to prevent the no-op calls on release.

@jkotas
Copy link
Member

jkotas commented Apr 1, 2020

@marek-safar Are you ok with keeping debug-only ParserState cruft in release builds, or do you prefer to have it conditionally excluded?

As I have said above, the value of these debug-only checks seems to be very low, and I believe it would be best to delete them.

@alexischr alexischr force-pushed the mono-port-coreclr-typenamebuilder branch from 6af771f to d7d5962 Compare April 1, 2020 17:49
…t/TypeNameBuilder.cs

Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
@jkotas
Copy link
Member

jkotas commented Apr 1, 2020

LGTM otherwise. Thanks!

…t/TypeNameBuilder.cs

Co-Authored-By: Marek Safar <marek.safar@gmail.com>
@alexischr alexischr force-pushed the mono-port-coreclr-typenamebuilder branch from 5434af8 to 6abeeb4 Compare April 2, 2020 14:15
@alexischr alexischr merged commit 3ab97fc into dotnet:master Apr 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants